Fix reused_stream losing healthy pooled connections (race with idle poller) - #1005
Open
sinhaparth5 wants to merge 1 commit into
Open
Fix reused_stream losing healthy pooled connections (race with idle poller)#1005sinhaparth5 wants to merge 1 commit into
sinhaparth5 wants to merge 1 commit into
Conversation
…oller) TransportConnector::reused_stream waited for the idle poller's OwnedMutexGuard to drop by briefly taking the stream's lock, then called Arc::try_unwrap assuming the strong count was back to 1. That races with tokio's OwnedMutexGuard::drop, which releases the semaphore *before* dropping its own Arc<Mutex<T>> clone: the waiter can be scheduled and reach try_unwrap while the poller's clone is still mid-drop. try_unwrap then returns Err, the connection is discarded, and the caller dials a new one even though the pooled connection was perfectly healthy. Fix by not depending on the Arc strong count at all: wrap the pooled stream in a new ReusableStream(Option<Stream>) and take() it out while still holding the mutex guard. Exclusive access to the lock is sufficient on its own to make the take() race-free, regardless of when the poller's leftover Arc clone finishes dropping. Fixes cloudflare#998
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #998.
Root cause
TransportConnector::reused_streamwaits for the idle poller'sOwnedMutexGuardto be released (by briefly locking the pooled stream's mutex), then callsArc::try_unwrap, assuming the strong count is back down to 1.That assumption races with
tokio::sync::OwnedMutexGuard::drop, which releases the semaphore permit before it drops its ownArc<Mutex<T>>clone (the permit release happens inside theDropimpl's body, and the guard'sArcfield is only dropped after that body returns). So there's a window where the waiter inreused_streamgets woken and scheduled on another thread before the poller's task has actually finished dropping itsArcclone. Iftry_unwrapruns in that window, it returnsErr, the healthy pooled connection gets thrown away, and the caller dials a new connection instead of reusing it.Nothing is corrupted or leaked. The only symptom is silently lost keepalive reuse, which on a busy proxy means extra TCP/TLS handshakes on the upstream path under load.
Fix
Stop depending on the
Arcstrong count. The pooled stream is now wrapped asArc<Mutex<ReusableStream>>, whereReusableStream(Option<Stream>), andreused_streamholds the lock guard andtake()s the stream out of theOptioninstead of callingArc::try_unwrap. Holding the mutex guard is enough on its own for exclusivity, regardless of whether the poller's leftoverArcclone has finished dropping yet.Testing
cargo test -p pingora-core --features rustlspasses.connectors::tests::test_connect_udsas 400 fresh processes (fresh multi-threaded runtime per iteration, 8-way parallel, matching the repro methodology in reused_stream can miss a pooled connection when try_unwrap races the idle poller #998) gave 11/400 (2.75%) failures on the parent commit and 0/400 on this fix, all at the sameassert!(reused)site described in the issue.